🤖 refactor: cut the taskService/workspaceService dependency cycle at a typed seam - #3996
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
a6bf1b2 to
3f94aa7
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
|
@codex review Branch reworked per reviewer directive: the seam now pays for itself. Net diff vs main is +676/−1010 (−334). New commits dedupe seam types into taskWorkspaceSeam.ts, encode port invariants in types instead of optional chaining, and delete seam-obsoleted test scaffolding (phantom-method guard, wiring-only tests, private-spy tests, copy-pasted mock blocks, dead stubs). |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This comment has been minimized.
This comment has been minimized.
_Generated with `xum` • Model: `openai:gpt-5.6-sol` • Thinking: `high`_ <!-- mux-attribution: model=openai:gpt-5.6-sol thinking=high -->
Review follow-up: the four per-command handlers are only reachable through processSlashCommand now, and /plan open lost its dedicated tests in the result-based rewrite. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P1: getDraft captured at command invocation reports that render's input, so async commands cleared newer drafts on consume and never fired restore-if-empty. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P2: text equality cannot distinguish a retyped identical draft from the original invocation. Commands already clear through their own clear-input actions (matching trunk), so the terminal clear was additive and could only destroy mid-phase drafts. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
Codex P2 follow-up: with the terminal consume-path clear gone, /dream and /refine left the executed command re-runnable in the composer. Emit clear-input from the handlers so commands own their composer effects. _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh -->
…itative in background wrappers
…te; drop dead code
The seam file is now the single documented home for ArchiveWorkspaceOptions, SendMessageInternalOptions, and WorkspaceLiveActivity instead of duplicating workspaceService declarations. StreamErrorRecoveryOutcome comes from its canonical agentSession export. The AgentTaskStatus re-export shim is gone; importers use the seam. Method-level optional chaining on both typed ports is removed: the interfaces guarantee the methods, so the narrow-mock hedges and their comments no longer apply.
… suite The typed AgentTaskIntegration port makes several tests unwritable or redundant: the phantom cleanupReportedDescendantsAfterArchive guard (method never existed in production), the archive lock pass-through wiring assertion, and four private updateAgentStatus non-invocation spies whose positive registerSession behavior tests remain. Near-identical send/resume lifecycle, winding-down, auto-resume, and foreground-wait-backgrounding siblings collapse into table-driven tests preserving every case, and dead fake stubs the code under test never reads are dropped.
…d host port createWorkspaceServiceMocks loses its dead stubs (waitForIdle, deleteWorktree, updateAgentStatus never existed on WorkspaceHost), its hand-written 37-line return annotation, and return entries nothing consumes; the removeQueuedMessagesByDedupeKeyPrefix override is wired instead of the one call site mutating the built host. 23 copy-pasted createWorkspace mock blocks collapse into two shared helpers, and the redundant conditional-spread forwarding in both harnesses becomes direct pass-through since the factory already defaults absent overrides.
Drops fake overrides that restate makeAgentTaskIntegrationFake defaults, types makeWorkspaceTurnCreateMock's rest args via Parameters<WorkspaceHost['create']> instead of an unknown[] cast, and calls withTaskTreeLifecycleLock directly rather than through a bound closure.
122fb5b to
e05dc05
Compare
…oder#4012) ## Summary Splits the 36-method `WorkspaceHost` grab-bag on the task/workspace seam into five role interfaces named for what task-side callers do (`WorkspaceTurnHost`, `TurnAdmissionHost`, `WorkspaceLifecycleHost`, `WorkspaceProvisioningHost`, `WorkspaceMetadataHost`), keeps `WorkspaceHost` as their intersection so no call site or wiring changes, and collapses the ~180-line hand-rolled test mock onto one shared `makeWorkspaceHostFake`. ## Background coder#3996 cut the taskService/workspaceService dependency cycle at a typed seam, but the seam stayed shallow: one interface mirroring 36 of WorkspaceService's internal mechanics. Every task test stubbed all 36 methods through a ~180-line mock in `taskService.test.ts` (reached from 318 call sites), and the seam, service, and test harness churned in lockstep on every change. This builds on coder#3996 rather than reverting it: same seam, deeper interface. Refactor coder#5 from the 2026-08-29 architecture review (evidence at main @ f04e0f8). ## Implementation - `taskWorkspaceSeam.ts`: the five role interfaces group methods by caller intent (turn execution, queue/admission probes, archive/remove lifecycle, child-workspace provisioning, metadata/events). Every method signature is byte-identical to before; `WorkspaceHost` is now `WorkspaceTurnHost & TurnAdmissionHost & WorkspaceLifecycleHost & WorkspaceProvisioningHost & WorkspaceMetadataHost`. `TaskService` (the only production consumer) legitimately uses all five roles, so its single constructor param stays; new narrow consumers can now depend on one role instead of the full host. - `taskWorkspaceSeam.testUtils.ts`: adds framework-free `makeWorkspaceHostFake(overrides)` beside the existing `makeAgentTaskIntegrationFake`, carrying the harness's default stub semantics (granted archive hold, "keep"-style snapshot eligibility, sanitizer no-op). - `taskService.test.ts`: `createWorkspaceServiceMocks` shrinks from ~180 lines to ~55 on top of the shared fake, with a mapped type over `keyof WorkspaceHost` replacing the hand-written 36-entry overrides list. Returned mock handles and the archive/remove locked-sink aliasing are preserved, so all 318 harness call sites are untouched. ## Net LOC delta vs main (f04e0f8) - Production (`taskWorkspaceSeam.ts`): **+18** (+103/-85) - Tests (`taskService.test.ts` + `taskWorkspaceSeam.testUtils.ts`): **-76** (+112/-188) - Overall: **-58** Irreducible production additions: the five role interface declarations plus the intersection type (the point of the refactor), and the archive race-invariant docs on `ArchiveWorkspaceOptions`, which review feedback correctly required keeping verbatim rather than counting as savings. Test additions are `makeWorkspaceHostFake`'s default bodies (moved from the harness, now reusable by any seam consumer's tests). ## Validation - Remote dogfood UAT ran against the pushed SHA and passed: sub-agent spawn/report/interrupt/reawaken, workspace turns (new + queued follow-up race), archive/unarchive including `interrupt_active` and live-activity refusal, heartbeats, tree listing, and monitor wakes. The UAT runner additionally verified the emitted JavaScript is byte-identical between base and feature for the production file. - Whole-file `bun test` of `taskService.test.ts`, `workspaceService.test.ts`, `heartbeatService.test.ts`, `tools/task_list.test.ts`: the fail set is identical to a clean worktree at base f04e0f8 (3 pre-existing host-environment failures; none branch-attributable). ## Risks Low. The production change is type-only interface restructuring with byte-identical emitted JS; regression surface is the test-harness consolidation, which preserves each mock's default behavior and aliasing semantics. --- _Generated with `xum` • Model: `anthropic:claude-fable-5` • Thinking: `xhigh` • Cost: `$29.31`_ <!-- mux-attribution: model=anthropic:claude-fable-5 thinking=xhigh costs=29.31 --> ## Stack Layer 4/10 of the architecture refactor stack (net -5,101 LOC overall). This PR's diff is only this layer, against `mike/arch-peer-message-broker`.
Summary
Cuts the TaskService/WorkspaceService concrete-class dependency cycle with one-directional typed ports, then pays for the new contract by deleting the code and test scaffolding the ports make obsolete. The full branch is a net simplification: +676/−1010 (net −334) across 11 files.
Background
TaskService and WorkspaceService imported each other's concrete classes, so neither side had a stated contract: every public member of each class was implicitly reachable from the other, and tests smuggled partial fakes through
as unknown ascasts. An architecture change must shrink the codebase, so the seam had to delete more than it declares.Implementation
taskWorkspaceSeam.tsis the single home for the two ports and every type they share:WorkspaceHost(~38 methods): what TaskService may call; WorkspaceServiceimplementsit.AgentTaskIntegration(12 methods): what WorkspaceService may call; TaskServiceimplementsit, wired viasetAgentTaskIntegration(the oldsetTaskServiceis deleted).ArchiveWorkspaceOptions,SendMessageInternalOptions(formerly a 56-line inline literal),WorkspaceLiveActivity,AgentTaskStatus, and the archive untracked-path helpers moved here instead of being duplicated;StreamErrorRecoveryOutcomeis imported from its canonicalagentSessionexport. TheAgentTaskStatusre-export shim in taskService is gone; importers use the seam.integration?.method?.()) and the "test harnesses mock a narrow surface" hedges are deleted because the interfaces now guarantee the methods exist.Test shrink (where the net-negative lives)
as unknown as TaskService/WorkspaceServicecasts:makeAgentTaskIntegrationFake()(shared testUtils) and asatisfies WorkspaceHostcentral factory.cleanupReportedDescendantsAfterArchiveguard (the method never existed in production), an archive lock pass-through assertion, and four privateupdateAgentStatusnon-invocation spies whose positive behavior tests remain.test.eachtables (send/resume interrupted-task lifecycle, winding-down gate, auto-resume reset, foreground-wait backgrounding); every case is preserved.createWorkspaceServiceMockslost its dead stubs (waitForIdle,deleteWorktree,updateAgentStatusnever existed on the port), its hand-written 37-line return annotation, and unconsumed return entries; 23 copy-pastedcreateWorkspacemock blocks collapsed into two shared helpers typed viaParameters<WorkspaceHost["create"]>; redundant conditional-spread forwarding in two harnesses became direct pass-through.Validation
make static-checkgreen; typecheck is the enforcement mechanism for the seam (it caught two false "unused symbol" audit claims during development).Risks
The production diff is receiver-renames, type moves, and deleted dead plumbing; runtime control flow is unchanged (one
.bindindirection replaced by a direct guarded call). Regression risk concentrates in the test consolidation: atest.eachtranslation error could weaken a race guard. Mitigated by preserving each original case row and assertion set, and by the suites passing with an unchanged test count on the task side.Generated with
xum• Model:anthropic:claude-fable-5• Thinking:xhigh